Fix tests with Go1.17 by QuLogic · Pull Request #1066 · exercism/cli

QuLogic · 2022-10-18T04:32:17Z

Part.FileName now applies filepath.Base to the return value [1]. For compatibility with older versions, this now explicitly applies Base when fetching FileName always.

Note that CI is on 1.15 which is no longer supported; I can update that to supported versions if needed.

[1] https://go.dev/doc/go1.17#mime/multipart

`Part.FileName` now applies `filepath.Base` to the return value [1]. For compatibility with older versions, this now explicitly applies `Base` when fetching `FileName` always. [1] https://go.dev/doc/go1.17#mime/multipart

iHiD · 2023-09-20T13:18:34Z

 		assert.Equal(t, 3, len(submittedFiles))
 		assert.Equal(t, "This is file 1.", submittedFiles["file-1.txt"])
-		assert.Equal(t, "This is file 2.", submittedFiles["subdir/file-2.txt"])
+		assert.Equal(t, "This is file 2.", submittedFiles["file-2.txt"])


Hey! I'm a bit unclear on this test changing. If someone submits:

file-1.txt subdir/file-2.txt file-2.txt

what happens? Have we lost that ability, or just lost a test for that? Or something else? :)

This change is just to fix the response in the test case, as the multipart form used in the test server now returns the filename without the directory information link to Go source

Using Go 1.20.8

This is the fileheader information that gets returned in the test. You can see the form-data is correct, which will be handled by the upstream API as is. But the returned Filename contains just the base filename, which is what is causing the test to fail.

=== RUN TestSubmitFiles { "Filename": "file-1.txt", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"file-1.txt\"" ], "Content-Type": [ "application/octet-stream" ] }, "Size": 15 }, { "Filename": "file-2.txt", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"subdir/file-2.txt\"" ], "Content-Type": [ "application/octet-stream" ] }, "Size": 15 }, { "Filename": "README.md", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"README.md\"" ], "Content-Type": [ "application/octet-stream" ] } --- FAIL: TestSubmitFiles (0.00s)

With the fix in place you see the fileheader information is still the same but the test is now just expecting the base filename so all is well.

=== RUN TestSubmitFiles { "Filename": "file-1.txt", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"file-1.txt\"" ], "Content-Type": [ "application/octet-stream" ] }, "Size": 15 }, { "Filename": "file-2.txt", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"subdir/file-2.txt\"" ], "Content-Type": [ "application/octet-stream" ] }, "Size": 15 }, { "Filename": "README.md", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"README.md\"" ], "Content-Type": [ "application/octet-stream" ] }, "Size": 19 } -- PASS: TestSubmitFiles (0.00s)

With the fix in place you see the fileheader information is still the same but the test is now just expecting the base filename so all is well.

I see, but we are losing information now, as we're no longer testing the subdir, which is really important. Can the test be restructured to allow testing the filename and the directory?

Ah that's a fair point. I was thinking solely from the point of the submitted file content and not the directory tree. We can pull the unmodified filename from the Header to validate instead of using fileHeader.Filename. Which looking at the Go PR where this change was introduced implies as the thing to do. I opened a PR with a fix that keeps the directory info intact. Refer to #1115

nywilken · 2023-09-20T13:59:57Z

 				t.Fatal(err)
 			}
-			submittedFiles[fileHeader.Filename] = string(body)
+			submittedFiles[filepath.Base(fileHeader.Filename)] = string(body)


So this particular change can be dropped once the Go version for running the test on CI is bumped from 1.15 to 1.17+, as fileheader.Filename will already have the directory information stripped from it.

So this particular change can be dropped once the Go version for running the test on CI is bumped from 1.15 to 1.17+, as fileheader.Filename will already have the directory information stripped from it.

Thanks :) Shall we do this as part of this PR?

Given this is a small change and ready to go. I would suggest merging as is and creating a new PR for bumping Go versions.

That said, since the go.mod is still at 1.15 we may not want to remove this if the expectation is to have Go 1.15 as the target toolchain version.

——
My rationale for a separate PR is so that we can look into adding a matrix to test against different Go versions during CI. This would be good to validate that new changes don’t break compatibly with older supported go versions. For example the CLI releases are made with 1.19 but the CI is 1.15. A matrix can help validate each version.

As alternative maybe @ErikSchierboom and @kytrinyx would be up for moving to a higher version of Go for dev and bumping the go.mod file version. Which also involves updating the contributing guide to call out the min dev version.

These are straight forward changes that won’t introduce any breaking changes but will have a little meat to them. So a separate PR feels right.

I agree that it makes sense to do it in a separate PR (and looking into a matrix seems sensible).

I don't have any objections to bumping the Go version (but also I don't actually do any real work on this anymore, so I don't have skin in the game).

@iHiD just a heads up I'm working on a PR to bump Go versions. I will work on getting it opened this week. It currently sits as a draft at nywilken#1

Do you prefer I open a discussion on the Exercism forum before opening a PR?

nywilken · 2023-09-20T14:00:00Z

 		assert.Equal(t, 3, len(submittedFiles))
 		assert.Equal(t, "This is file 1.", submittedFiles["file-1.txt"])
-		assert.Equal(t, "This is file 2.", submittedFiles["subdir/file-2.txt"])
+		assert.Equal(t, "This is file 2.", submittedFiles["file-2.txt"])


This change is just to fix the response in the test case, as the multipart form used in the test server now returns the filename without the directory information link to Go source

Using Go 1.20.8

This is the fileheader information that gets returned in the test. You can see the form-data is correct, which will be handled by the upstream API as is. But the returned Filename contains just the base filename, which is what is causing the test to fail.

=== RUN TestSubmitFiles { "Filename": "file-1.txt", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"file-1.txt\"" ], "Content-Type": [ "application/octet-stream" ] }, "Size": 15 }, { "Filename": "file-2.txt", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"subdir/file-2.txt\"" ], "Content-Type": [ "application/octet-stream" ] }, "Size": 15 }, { "Filename": "README.md", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"README.md\"" ], "Content-Type": [ "application/octet-stream" ] } --- FAIL: TestSubmitFiles (0.00s)

With the fix in place you see the fileheader information is still the same but the test is now just expecting the base filename so all is well.

=== RUN TestSubmitFiles { "Filename": "file-1.txt", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"file-1.txt\"" ], "Content-Type": [ "application/octet-stream" ] }, "Size": 15 }, { "Filename": "file-2.txt", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"subdir/file-2.txt\"" ], "Content-Type": [ "application/octet-stream" ] }, "Size": 15 }, { "Filename": "README.md", "Header": { "Content-Disposition": [ "form-data; name=\"files[]\"; filename=\"README.md\"" ], "Content-Type": [ "application/octet-stream" ] }, "Size": 19 } -- PASS: TestSubmitFiles (0.00s)

iHiD · 2023-09-21T17:06:19Z

OK, cool. Thank you for the reviews and feedback! Let's get this merged then. @ErikSchierboom Could you lead on this pls?

ErikSchierboom · 2023-09-22T06:32:35Z

 		assert.Equal(t, 3, len(submittedFiles))
 		assert.Equal(t, "This is file 1.", submittedFiles["file-1.txt"])
-		assert.Equal(t, "This is file 2.", submittedFiles["subdir/file-2.txt"])
+		assert.Equal(t, "This is file 2.", submittedFiles["file-2.txt"])


With the fix in place you see the fileheader information is still the same but the test is now just expecting the base filename so all is well.

I see, but we are losing information now, as we're no longer testing the subdir, which is really important. Can the test be restructured to allow testing the filename and the directory?

@QuLogic

Following RFC 7578, Go 1.17+ strips the directory information in fileHeader.Filename. This change updates the test server to use Header["Content-Disposition"] for obtaining the unmodified filename for validating the submitted files directory tree in the request. This work builds on the PR opened by @QuLogic exercism#1066

@QuLogic

) Following RFC 7578, Go 1.17+ strips the directory information in fileHeader.Filename. This change updates the test server to use Header["Content-Disposition"] for obtaining the unmodified filename for validating the submitted files directory tree in the request. This work builds on the PR opened by @QuLogic #1066 Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>

ErikSchierboom · 2023-09-22T13:38:36Z

Closed by #1115

Fix tests with Go1.17

7ca0ddd

`Part.FileName` now applies `filepath.Base` to the return value [1]. For compatibility with older versions, this now explicitly applies `Base` when fetching `FileName` always. [1] https://go.dev/doc/go1.17#mime/multipart

nywilken mentioned this pull request Sep 19, 2023

Add list command #1113

Closed

iHiD reviewed Sep 20, 2023

View reviewed changes

nywilken reviewed Sep 20, 2023

View reviewed changes

iHiD requested a review from ErikSchierboom September 21, 2023 17:05

ErikSchierboom requested changes Sep 22, 2023

View reviewed changes

nywilken mentioned this pull request Sep 22, 2023

cmd/submit_test.go: Update test server to use unmodified filename #1115

Merged

ErikSchierboom closed this Sep 22, 2023

QuLogic deleted the fix-go117 branch September 22, 2023 17:13

Uh oh!

Conversation

QuLogic commented Oct 18, 2022

Uh oh!

iHiD Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

nywilken Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikSchierboom Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

nywilken Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nywilken Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

iHiD Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

nywilken Sep 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kytrinyx Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

nywilken Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nywilken Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iHiD commented Sep 21, 2023

Uh oh!

ErikSchierboom Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

ErikSchierboom commented Sep 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nywilken Sep 20, 2023 •

edited

Loading

nywilken Sep 22, 2023 •

edited

Loading

nywilken Sep 21, 2023 •

edited

Loading

nywilken Oct 2, 2023 •

edited

Loading

nywilken Sep 20, 2023 •

edited

Loading